Manual CPFP #413

Zshan0 · 2022-06-04T19:45:49Z

This PR is to maintain code corresponding to the RFC that is being discussed for the addition of manual CPFP command as a JSONRPC API in revaultd. RFC is present here. #411 is the corresponding milestone tracker

Zshan0 · 2022-06-18T13:21:55Z

A simple test for cpfp has been written, I'm not sure what other kinds of tests are required. Please let me know

Zshan0 · 2022-06-25T12:17:04Z

I am unable to figure out a simple way of testing CPFPing multiple SpendTransactions, if there is a simpler scheme that I can follow please let me know.

The Current CPFP is able to terminate invalid txid with an RpcError but it fails to terminate for txid that is valid but is unknown, rather than relying on direct database functions to fetch the given txid and then throw an error if it is unable to find, is there a function that is specifically designed to check if a txid exists in the database that I can use for sanity checking before passing it further?

src/bitcoind/mod.rs

darosior

I had a look at the test for manual CPFP of multiple Spend transactions, which looks valid but cluttered.

tests/test_rpc.py

darosior · 2022-07-18T13:41:40Z

Is this ready for review?

darosior

Just some comments on the implementation for now. I'll review the tests tomorrow.

Why do you update the tests/servers/ submodules?

doc/API.md

src/bitcoind/mod.rs

doc/API.md

src/bitcoind/mod.rs

darosior

The message handler in the bitcoind thread is wrong. It would return which would stop the loop, and obviously, make the program crash on shutdown or any further tentative to use the bitcoind thread (such as for another manual CPFP). I guess the reason you have so many small tests is a workaround for this bug...

tests/test_rpc.py

src/bitcoind/mod.rs

src/bitcoind/poller.rs

src/bitcoind/mod.rs

tests/test_rpc.py

darosior · 2022-09-19T09:31:59Z

Untested, but i think you can use this to externally fund the CPFP wallet:

diff --git a/tests/test_framework/revault_network.py b/tests/test_framework/revault_network.py
index 0a840f8..8dfa68e 100644
--- a/tests/test_framework/revault_network.py
+++ b/tests/test_framework/revault_network.py
@@ -3,6 +3,7 @@ import logging
 import os
 import random
 
+from bip380.descriptors import Descriptor
 from ephemeral_port_reserve import reserve
 from nacl.public import PrivateKey as Curve25519Private
 from test_framework import serializations
@@ -541,6 +542,13 @@ class RevaultNetwork:
 
         return created_vaults
 
+    def fund_cpfp(self, amount):
+        """Send a coin worth this value to the CPFP descriptor"""
+        der_desc = Descriptor.from_str(str(self.cpfp_desc))
+        der_desc.derive(0)
+        decoded = self.bitcoind.rpc.decodescript(der_desc.script_pubkey.hex())
+        self.bitcoind.rpc.sendtoaddress(decoded["address"], amount)
+
     def secure_vault(self, vault):
         """Make all stakeholders share signatures for all revocation txs"""
         deposit = f"{vault['txid']}:{vault['vout']}"

darosior · 2022-09-19T09:36:56Z

This should fix the unit tests CI, and also fixes the configuration of the functional tests CI (thanks for reporting it!): #423.

Zshan0 · 2022-10-01T03:57:21Z

src/bitcoind/utils.rs

+        match tx.cpfp_txin(cpfp_descriptor, &revaultd.secp_ctx) {
+            Some(txin) => txins.push(txin),
+            None => {
+                log::error!("No CPFP txin for tx '{}'", tx.txid());
+                return Ok(txids.into_iter().collect());
+            }
+        }
+    }


Rather than returning all the transactions, I need to throw an error here. I'm not sure which one

You can create a new enum variant for it if none other fit.

Zshan0 · 2022-10-01T04:04:10Z

tests/test_rpc.py

+def get_unvault_txids(wallet, vaults):
+    unvault_txids = []
+    for vault in vaults:
+        deposit = f"{vault['txid']}:{vault['vout']}"
+        unvault_psbt = serializations.PSBT()
+        unvault_b64 = wallet.rpc.listpresignedtransactions([deposit])[
+            "presigned_transactions"
+        ][0]["unvault"]
+        unvault_psbt.deserialize(unvault_b64)
+        unvault_psbt.tx.calc_sha256()
+        unvault_txids.append(unvault_psbt.tx.hash)
+    return unvault_txids


Is it a good practice to import functions from other test files? This function is present in test_misc.py. I can also move it into a different file. Let me know what would be the best approach

I would make it a method of the RevaultNetwork class.

Along with tests.

Zshan0 force-pushed the jsonrpc_test branch 6 times, most recently from 64efaf3 to cdc6c11 Compare June 11, 2022 09:31

Zshan0 force-pushed the jsonrpc_test branch 3 times, most recently from 645a767 to d4c0832 Compare June 18, 2022 05:44

Zshan0 marked this pull request as ready for review June 18, 2022 13:22

Zshan0 force-pushed the jsonrpc_test branch 4 times, most recently from 39be8bd to 1d90a44 Compare June 25, 2022 12:12

darosior reviewed Jun 27, 2022

View reviewed changes

src/bitcoind/mod.rs Outdated Show resolved Hide resolved

darosior reviewed Jun 28, 2022

View reviewed changes

tests/test_rpc.py Outdated Show resolved Hide resolved

tests/test_rpc.py Outdated Show resolved Hide resolved

tests/test_rpc.py Outdated Show resolved Hide resolved

tests/test_rpc.py Outdated Show resolved Hide resolved

tests/test_rpc.py Outdated Show resolved Hide resolved

Zshan0 force-pushed the jsonrpc_test branch 3 times, most recently from 446283c to f217739 Compare July 3, 2022 11:18

Zshan0 force-pushed the jsonrpc_test branch 2 times, most recently from 2df277a to aa275ff Compare July 18, 2022 11:14

Zshan0 mentioned this pull request Jul 22, 2022

Additional tests #420

Draft

darosior reviewed Jul 31, 2022

View reviewed changes

darosior reviewed Aug 1, 2022

View reviewed changes

Zshan0 force-pushed the jsonrpc_test branch 2 times, most recently from 66048ee to a81036a Compare August 18, 2022 20:11

Zshan0 force-pushed the jsonrpc_test branch 4 times, most recently from e0ac86a to 6b497b4 Compare August 22, 2022 08:38

Zshan0 force-pushed the jsonrpc_test branch from 6b497b4 to 8710e1b Compare September 14, 2022 16:45

Zshan0 force-pushed the jsonrpc_test branch from 8710e1b to 32d0599 Compare September 22, 2022 09:38

Zshan0 commented Oct 1, 2022

View reviewed changes

Addition of new RPC command for manual cpfp.

a011fea

Along with tests.

Zshan0 force-pushed the jsonrpc_test branch from 32d0599 to a011fea Compare October 1, 2022 04:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manual CPFP #413

Manual CPFP #413

Zshan0 commented Jun 4, 2022 •

edited

Loading

Zshan0 commented Jun 18, 2022

Zshan0 commented Jun 25, 2022

darosior left a comment

darosior commented Jul 18, 2022

darosior left a comment

darosior left a comment

darosior commented Sep 19, 2022

darosior commented Sep 19, 2022

Zshan0 Oct 1, 2022

darosior Oct 3, 2022

Zshan0 Oct 1, 2022

darosior Oct 3, 2022

Manual CPFP #413

Are you sure you want to change the base?

Manual CPFP #413

Conversation

Zshan0 commented Jun 4, 2022 • edited Loading

Zshan0 commented Jun 18, 2022

Zshan0 commented Jun 25, 2022

darosior left a comment

Choose a reason for hiding this comment

darosior commented Jul 18, 2022

darosior left a comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

darosior commented Sep 19, 2022

darosior commented Sep 19, 2022

Zshan0 Oct 1, 2022

Choose a reason for hiding this comment

darosior Oct 3, 2022

Choose a reason for hiding this comment

Zshan0 Oct 1, 2022

Choose a reason for hiding this comment

darosior Oct 3, 2022

Choose a reason for hiding this comment

Zshan0 commented Jun 4, 2022 •

edited

Loading